Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lofs fid improvements #35

Merged
merged 11 commits into from
Nov 30, 2015
Merged

lofs fid improvements #35

merged 11 commits into from
Nov 30, 2015

Conversation

djs55
Copy link
Member

@djs55 djs55 commented Nov 29, 2015

  • walk should fail if the newfid is in use (if it is not the same as fid)
  • remove should clunk the fid in the failure case
  • associate open fid with file or directory handles
  • read and write of a closed fid now return bad fid
  • handle out-of-bounds directory reading a bit better

Walk(9P) says: "The walk request carries as arguments an existing fid and a
proposed newfid (which must not be in use unless it is the same as fid"

This patch returns an error if the newfid is already in use, unless it
is the same as fid.

Signed-off-by: David Scott <dave.scott@unikernel.com>
The Remove(9P) man page says: "The remove request asks the file server
both to remove the file represented by fid and to clunk the fid, even if
the remove fails."

This patch includes a unit test which checks this behaviour.

Signed-off-by: David Scott <dave.scott@unikernel.com>
This is the start of a series of patches where we associate more
server-side state with fids.

Signed-off-by: David Scott <dave.scott@unikernel.com>
Note we are not yet storing these open handles.

Signed-off-by: David Scott <dave.scott@unikernel.com>
Signed-off-by: David Scott <dave.scott@unikernel.com>
@djs55
Copy link
Member Author

djs55 commented Nov 30, 2015

@dsheets this is my first cut of associating handles with fids. Let me know what you think.

In the case I'm most concerned about today I don't know if this is a good enough fix. If I open a directory (opendir) and cache the dir_handle, and then on every read I do a with_mutex (rewinddir; readdir), will I see the contents changing if someone removes a file from the directory? I wonder if I need to actively snapshot the directory contents?

@djs55
Copy link
Member Author

djs55 commented Nov 30, 2015

I just managed to rm -rf something successfully - a positive sign

@dsheets
Copy link
Member

dsheets commented Nov 30, 2015

Cool! Great that rm -rf works now.

Note this means that the unit tests need to create the directories with
`Read permission, for them to be successfully opened.

This also implies that we perform an implicit mode check on `open` which
we didn't do before.

Signed-off-by: David Scott <dave.scott@unikernel.com>
Rather than opening fresh handles each time, we now use the handles from
`open` or `create`. Note this means we have to hold a mutex around
`seek`+`read` and `seek`+`write` to prevent parallel operations
conflicting over the file position.

Signed-off-by: David Scott <dave.scott@unikernel.com>
If the client reads beyond the end of the directory, return with a
length of 0 rather than an `Invalid_argument "Cstruct.sub..."`

Signed-off-by: David Scott <dave.scott@unikernel.com>
A symlink can be dangling, which means that `stat` will return `ENOENT`.
When removing we want to remove the symlink itself so `lstat` is
appropriate.

Signed-off-by: David Scott <dave.scott@unikernel.com>
Signed-off-by: David Scott <dave.scott@unikernel.com>
This avoids duplicating the `clunk` internals (i.e. closing the
associated Resource)

Signed-off-by: David Scott <dave.scott@unikernel.com>
@djs55
Copy link
Member Author

djs55 commented Nov 30, 2015

Thanks for the feedback. I've incorporated everything except I still need to rewrite the readdir use along the lines you suggested-- for expediency I'll probably make that one an issue and merge this PR since it makes things work a lot better with Linux.

djs55 added a commit that referenced this pull request Nov 30, 2015
@djs55 djs55 merged commit 361fd59 into mirage:master Nov 30, 2015
@djs55 djs55 deleted the fix-test branch November 30, 2015 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants